-
Notifications
You must be signed in to change notification settings - Fork 36
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add QualificationModal component #2727
Conversation
BREAKING CHANGE: You must have `cozy-client >= 51.6.0` and `cozy-intent >= 2.29.1`
fea50f0
to
a062f7a
Compare
BundleMonFiles updated (1)
Unchanged files (2)
Total files change +191B +0.54% Groups updated (1)
Final result: ✅ View report in BundleMon website ➡️ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
react/QualificationModal/index.jsx
Outdated
const fileCollection = client.collection('io.cozy.files') | ||
const removeQualification = qualificationLabel && id === 'none' | ||
|
||
if (!qualificationLabel && removeQualification) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dans removeQualification
tu vérifies la valeur booléene qualificationLabel
et là tu vérifie juste à côté !qualificationLabel
ça a pas trop de sens non ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Merkur39 tu te rappelles du pourquoi du comment ? (ce composant est un copier/coller d'une autre app avec un refacto pour simplifier, mais je n'ai pas touché au handleClick par exemple)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My bad, en effet il semble que cette condition ne soit jamais vrai 🤔
Je pense que seul removeQualification
est suffisant non ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Je comprends pas à quoi sert ce return onClose
en fait ? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Il redirigeait à l'origine, ici il hide la modal, mais oui je pense que toute cette condition n'est pas/plus utile 🤔
A confirmer, je loupe peut-être un cas spécifique ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
c'est pour fermer la modal sans rien faire.
Mais même pour "remove" la qualif, c'est à dire cliquer sur "aucun", il faut quand même réellement enlever la qualification je pense. En revanche ça pose la question des metadata 🤔 est-ce qu'on les garde ou non dans ce cas.
Peut-être que dans l'app d'où provient le code, on souhaiter traiter le cas "none" différemment... edit j'ai retrouvé ça vient de https://github.com/cozy/cozy-drive/blob/master/src/modules/views/Modal/QualifyFileView.jsx
ah non c'est pas ça, plus bas on a
await fileCollection.updateMetadataAttribute(file._id, {
qualification: removeQualification ? null : item
})
donc on savait qu'on passerait dedans en cas de remove...
Bon je supprime la condition
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Il faut conserver les metadata, il peut y en avoir d'autres que la qualif 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
je voulais dire les metadata associés à la qualif
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, je ne pense pas non plus, il me semble qu'elles font quand même sens même sans qualif
a062f7a
to
8b24934
Compare
🎉 This PR is included in version 114.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
No description provided.